Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add default_admin_services node #7077

Merged

Conversation

VincentLanglet
Copy link
Member

@VincentLanglet VincentLanglet commented Apr 16, 2021

Subject

BC

Changelog

### Added
- Added `default_admin_services` config key to overwrite the default services provided to all the admin.

### Deprecated
- Deprecated `admin_services` config key.

@jordisala1991
Copy link
Member

One question: Why dont you decorate the service you want to modify? It is a feature available at the symfony dependency injection component and it allows to not write more code to do the same

@VincentLanglet
Copy link
Member Author

One question: Why dont you decorate the service you want to modify? It is a feature available at the symfony dependency injection component and it allows to not write more code to do the same

I don't always want to modify a service.
For instance, #4637 ; you don't want to decorate the default label translator strategy ; just using another one.

@VincentLanglet VincentLanglet force-pushed the default_admin_services branch 2 times, most recently from c209e99 to d94691a Compare April 19, 2021 20:31
@VincentLanglet VincentLanglet requested a review from a team April 19, 2021 20:34
@VincentLanglet VincentLanglet marked this pull request as ready for review April 19, 2021 20:34
@VincentLanglet
Copy link
Member Author

Please review @sonata-project/contributors

@VincentLanglet
Copy link
Member Author

I will update the tests after #7127

@VincentLanglet VincentLanglet marked this pull request as draft April 27, 2021 20:20
@VincentLanglet VincentLanglet force-pushed the default_admin_services branch 2 times, most recently from 0c72528 to 2e1bc4d Compare April 30, 2021 20:46
@VincentLanglet VincentLanglet marked this pull request as ready for review April 30, 2021 20:47
@VincentLanglet
Copy link
Member Author

Please review @sonata-project/contributors

@franmomu
Copy link
Member

franmomu commented May 2, 2021

If the use case was to select one of the available label translation strategies, what about to just add an option to set the default one?

@VincentLanglet
Copy link
Member Author

If the use case was to select one of the available label translation strategies, what about to just add an option to set the default one?

What's the issue with the option default_admin_services.label_translator_strategy I added ?

Other use case can be:

  • Setting the default form theme to use default_admin_services.templates.form
  • Setting the default pager type to use default_admin_services.pager_type
  • Setting the default data source to use default_admin_services.data_source
    And so on.

@franmomu
Copy link
Member

franmomu commented May 2, 2021

IMO for modifying existing services you can do it with a service decorator as @jordisala1991 said, I think we don't need to provide a way to override them (it is the way it's documented in Symfony for overriding services from a bundle).

For templates we already have a sonata_admin.templates configuration that I think we can use instead of creating another global one.

@VincentLanglet
Copy link
Member Author

VincentLanglet commented May 2, 2021

IMO for modifying existing services you can do it with a service decorator as @jordisala1991 said, I think we don't need to provide a way to override them (it is the way it's documented in Symfony for overriding services from a bundle).

I may not understanding correctly decoration, but I don't think it solves the issue.
It's not about modifying a service, it's about choosing another one.

In #4637, the goal is to use a different label_translator_strategy.
It can be either one provided by sonata, either one custom.
It would avoid configuring label_translator_strategy to every admin.

For pager_type, you can use default or simple.
If you want to use the simple for every admin, this is the only way to do it.

Another example: I wrote a whole custom datasource service and I'd like to use it for almost all my admin.
I don't want to decorate the existing datasource service, my datasource has nothing in config with the existing one.

For templates we already have a sonata_admin.templates configuration that I think we can use instead of creating another global one.

We can add form, filter, view keys in sonata_admin.templates if you prefer.
I just followed the previous was it was done.

@franmomu
Copy link
Member

franmomu commented May 2, 2021

What I meant is:

  • If we want to override a service, it can be done with service decoration.
  • If we have several choices, we can define the default one and provide an option for that.

I may not understanding correctly decoration, but I don't think it solves the issue.
It's not about modifying a service, it's about choosing another one.

In #4637, the goal is to use a different label_translator_strategy.
It can be either one provided by sonata, either one custom.
It would avoid configuring label_translator_strategy to every admin.

For example in DoctrineBundle:

doctrine:
    dbal:
        default_connection: default
    orm:
        default_entity_manager: ~

We could add:

sonata_admin:
    default_label_strategy: 'sonata.admin.label.strategy.native' # by default

This way, instead of:

'label_translator_strategy' => 'sonata.admin.label.strategy.native',

could be

'label_translator_strategy' => 'sonata.admin.label.strategy.default',

and sonata.admin.label.strategy.default would be an alias to the service specified in sonata_admin.default_label_stategy.

If for example we create a service that receives the label translation strategy, this way we could inject 'sonata.admin.label.strategy.default'.

For pager_type, you can use default or simple.
If you want to use the simple for every admin, this is the only way to do it.

In this case (I know it was that way, but this is a new feature) sonata_admin.default_admin_services.pager_type I would expect a service and in this case it's a choice (default, simple or other custom one), so not sure if it's in the right place or naming.

Another example: I wrote a whole custom datasource service and I'd like to use it for almost all my admin.
I don't want to decorate the existing datasource service, my datasource has nothing in config with the existing one.

When decorating the service there is no need to configure it with the existing one, you can use it or not.

For templates we already have a sonata_admin.templates configuration that I think we can use instead of creating another global one.

We can add form, filter, view keys in sonata_admin.templates if you prefer.
I just followed the previous was it was done.

IMHO if we already have a configuration for templates (and if it's possible), we should use it, AFAIK the view key it is not needed because those templates are the ones in sonata_admin.templates.

@VincentLanglet
Copy link
Member Author

What I meant is:

  • If we want to override a service, it can be done with service decoration.

When I read the documentation

# config/services.yaml
services:
    App\Mailer: ~

    # this replaces the old App\Mailer definition with the new one, the
    # old definition is lost
    App\Mailer:
        class: App\NewMailer

The old definition is lost.

When decorating the service there is no need to configure it with the existing one, you can use it or not.

You might want to use your custom service 90% of the time and still keep the old definition sometimes.
So decoration doesn't solve the issue here, isn't it ?

In this case (I know it was that way, but this is a new feature) sonata_admin.default_admin_services.pager_type I would expect a service and in this case it's a choice (default, simple or other custom one), so not sure if it's in the right place or naming.

I just followed the existing feature, and the way it's done for attributes of the tag sonata.admin.

        tags:
            - name: sonata.admin
            - pager_type: default
            - label_translator_strategy: sonata.admin.label.strategy.native

public function applyConfigurationFromAttribute(Definition $definition, array $attributes)

$pagerType = $overwriteAdminConfiguration['pager_type'] ?? $attributes['pager_type'] ?? Pager::TYPE_DEFAULT;

IMHO we should keep consistency between the sonata_admin config and the sonata.admin tag attributes.

What I have in mind is to allow the user to change the default values of the sonata.admin tag attributes.
Maybe I can change the name of default_admin_services ? @franmomu

@VincentLanglet VincentLanglet force-pushed the default_admin_services branch 4 times, most recently from f25d0d9 to b735fa7 Compare May 2, 2021 13:48
@VincentLanglet
Copy link
Member Author

@franmomu @jordisala1991 Can you take a new look ?

  • I moved the template part to the already existing template key.
  • Does another name than default_admin_services could make a consensus ?

@VincentLanglet VincentLanglet requested a review from franmomu May 6, 2021 23:34
@VincentLanglet VincentLanglet force-pushed the default_admin_services branch from d885f96 to 4f70788 Compare May 7, 2021 07:35
@VincentLanglet
Copy link
Member Author

Can I get some review @sonata-project/contributors

jordisala1991
jordisala1991 previously approved these changes May 8, 2021
Comment on lines +451 to +456
->arrayNode('form_theme')
->prototype('scalar')->end()
->end()
->arrayNode('filter_theme')
->prototype('scalar')->end()
->end()
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should these new values be documented somewhere?

@VincentLanglet
Copy link
Member Author

I updated the doc :)

@VincentLanglet VincentLanglet requested a review from franmomu May 8, 2021 17:32
@VincentLanglet VincentLanglet merged commit 6ec9a7c into sonata-project:3.x May 8, 2021
@VincentLanglet VincentLanglet deleted the default_admin_services branch May 8, 2021 20:23
@peter-gribanov
Copy link
Contributor

Perhaps it would be worthwhile to somehow more explicitly state that this change breaks BC, and give an upgrade example.

-sonata_doctrine_orm_admin:
-    templates:
-        form:
-            - '@Frontend/Admin/form/doctrine_orm_admin_fields.html.twig'
-            - '@Frontend/Admin/form/admin_fields.html.twig'
-        filter:
-            - '@Frontend/Admin/form/filter_fields.html.twig'
+sonata_admin:
+    templates:
+        form_theme:
+            - '@Frontend/Admin/form/doctrine_orm_admin_fields.html.twig'
+            - '@Frontend/Admin/form/admin_fields.html.twig'
+        filter_theme:
+            - '@Frontend/Admin/form/filter_fields.html.twig'

@VincentLanglet
Copy link
Member Author

Perhaps it would be worthwhile to somehow more explicitly state that this change breaks BC, and give an upgrade example.

-sonata_doctrine_orm_admin:
-    templates:
-        form:
-            - '@Frontend/Admin/form/doctrine_orm_admin_fields.html.twig'
-            - '@Frontend/Admin/form/admin_fields.html.twig'
-        filter:
-            - '@Frontend/Admin/form/filter_fields.html.twig'
+sonata_admin:
+    templates:
+        form_theme:
+            - '@Frontend/Admin/form/doctrine_orm_admin_fields.html.twig'
+            - '@Frontend/Admin/form/admin_fields.html.twig'
+        filter_theme:
+            - '@Frontend/Admin/form/filter_fields.html.twig'

This PR is not supposed to break BC.
The old way to set templates is still available in 3.x.
The release 3.98.2 introduced an issue, fixed in SonataDoctrineORMAdminBundle/[email protected].

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants